-
Notifications
You must be signed in to change notification settings - Fork 220
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🏗️ Move wasm-smith's non-trapping mode logic to CodeBuilder #769
🏗️ Move wasm-smith's non-trapping mode logic to CodeBuilder #769
Conversation
5368d5d
to
4a4b4a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, although yeah we should have that fuzz target
|
||
/// Mask data and element segments' offsets to be in bounds of their | ||
/// associated tables and memories. | ||
fn no_trapping_segments(&mut self) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i just realized I never moved this over
if module.config.disallow_traps() { | ||
no_traps::store( | ||
Instruction::V128Store { memarg }, | ||
module, | ||
builder, | ||
instructions, | ||
); | ||
} else { | ||
instructions.push(Instruction::V128Store { memarg }); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m wondering if this pattern would be better served by making a choice between trapping or non-trapping variants and then generating an instruction that is guaranteed to be either way? Are there any instructions that would fall in the “unknown” territory somewhere?
This sort of split model would allow to easily add/maintain other kinds of generation “modes” as well (e.g. deterministic vs non-deterministic), and I wonder if it would result in better fuzz coverage (the fuzzer would be able to flip between the two broad categories of instructions at runtime on a per-instruction basis)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nagisa that's interesting! I'm wondering how useful the "guaranteed to trap" case would actually be. If we were to create a version of this code that does the opposite (ie. makes every division have a denominator of 0, makes every load/store have an out of bounds address, etc), then I would think any generated programs would trap relatively early and not improve coverage all that much except in testing that traps are handled correctly by the runtime. Maybe a wasm-smith flag to generate programs that are guaranteed to eventually trap would be helpful for improving coverage, since that would allow us to explore whether different program state affects the trap-handling behavior of the runtime.
Either way, maybe we discuss this further on #266 (or a new issue)? I think work in that direction could be an improvement on this work, but not immediately necessary.
As far as ease of adding/maintaining other modes, I agree that having this turn into something like the below would be unideal:
if option_1 {
generate_instruction();
} else if option_2 {
generate_instruction_but_slightly_differently();
} else {
generate_instruction_more_differently();
}
That being said, I think I'm inclined to leave that kind of refactoring for when/if we actually add more modes like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That being said, I think I'm inclined to leave that kind of refactoring for when/if we actually add more modes like this
Of course, that was just me thinking out loud, not necessarily suggesting that this must be implemented here and now ^^ Should have made it clear in my original comment.
@fitzgen I've been finagling with the fuzzer for a little bit and I haven't been able to get any functions to run yet, but I think I might be close(🤞). The fuzzer currently errs on line 49 with
I'm not sure where that size is coming from, but I think it's in the Memory being generated by |
fuzz/fuzz_targets/no-traps.rs
Outdated
config.threads_enabled = false; | ||
config.allow_start_export = false; | ||
config.max_memories = config.max_memories.min(1); | ||
config.memory64_enabled = true; | ||
config.exceptions_enabled = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These config options were mostly an attempt to get around errors that were preventing the Module or Store from getting instantiated. I would not be surprised if some of these aren't actually necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good, a few suggestions below. Is it running and passing?
fuzz/Cargo.toml
Outdated
path = "fuzz_targets/no-traps.rs" | ||
test = false | ||
doc = false | ||
required-features = ["wasmtime"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: no newline at the end of the file.
fuzz/fuzz_targets/no-traps.rs
Outdated
let (wasm_bytes, config) = match wasm_tools_fuzz::generate_valid_module(&mut u, |config, _| { | ||
config.disallow_traps = true; | ||
config.threads_enabled = false; | ||
config.allow_start_export = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be necessary.
fuzz/fuzz_targets/no-traps.rs
Outdated
config.threads_enabled = false; | ||
config.allow_start_export = false; | ||
config.max_memories = config.max_memories.min(1); | ||
config.memory64_enabled = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be necessary either.
fuzz/fuzz_targets/no-traps.rs
Outdated
config.disallow_traps = true; | ||
config.threads_enabled = false; | ||
config.allow_start_export = false; | ||
config.max_memories = config.max_memories.min(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not this one actually.
fuzz/fuzz_targets/no-traps.rs
Outdated
store.add_fuel(1_000).unwrap(); | ||
match func.call(&mut store, &args, &mut results) { | ||
Ok(_) => continue, | ||
Err(_) => panic!("wasm trapped"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trap might be because we ran out of fuel, in which case it is fine and we can continue.
fuzz/fuzz_targets/no-traps.rs
Outdated
let mut results = dummy::dummy_values(func_ty.results()); | ||
let func = instance.get_func(&mut store, export.name()).unwrap(); | ||
func_ty.results(); | ||
store.add_fuel(1_000).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want to reset to exactly 1000 fuel each time, rather than keep adding 1000 in the case where there is left over fuel from last iteration.
fuzz/fuzz_targets/no-traps.rs
Outdated
// Validate the module or component and assert that it passes validation. | ||
let mut validator = wasmparser::Validator::new_with_features(wasmparser::WasmFeatures { | ||
component_model: false, | ||
multi_value: config.multi_value_enabled, | ||
multi_memory: config.max_memories > 1, | ||
bulk_memory: true, | ||
reference_types: true, | ||
simd: config.simd_enabled, | ||
relaxed_simd: config.relaxed_simd_enabled, | ||
memory64: config.memory64_enabled, | ||
threads: config.threads_enabled, | ||
exceptions: config.exceptions_enabled, | ||
..wasmparser::WasmFeatures::default() | ||
}); | ||
if let Err(e) = validator.validate_all(&wasm_bytes) { | ||
panic!("Invalid module: {}", e); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be a little bit easier to read this fuzz target if validation was pulled out into a validate
function and then calling exports was also pulled out into its own call_function_exports
function or something like that. As is, it is a bit of a wall of code to read.
We can configure a maximum to the size of memories generated to avoid this issue. |
No not yet! As-is it bails at line 49 every time so it isn't even making it to the function calls. I'll remove the config options that aren't needed but I might need some help to figure out how to properly instantiate the wasm. I roughly followed the pattern in the failed-instantiations fuzzer, but I think I'm missing something |
d6cf31b
to
12b440a
Compare
a28e99d
to
9eb28a0
Compare
I've spent some time these past couple days tracking down some edge cases in the non-trapping logic. My general workflow for this has been:
The amount of time spent at step 2 is increasing (which is good!), but due to the nature of this, it's hard to know if there are 3 more bugs or 300 more bugs. A good "readiness" measure here might be "fuzzer runs for X minutes without crashing" or even better "the fuzz target is starting to find issues elsewhere". For now, I'm going to step away for a day or so and continue plugging away at this later. |
Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
in non-trapping mode
Co-authored-by: Nick Fitzgerald <fitzgen@gmail.com>
It was occasionally running into an 'attempt to multiply with overflow' for 32-bit memories, so we check the value before casting to avoid that.
for using eq, but it is the only value that is not equal to itself, so ne will work.
an f64. This is necessary because `i64::MAX as f64` will result in an f64 value that is not representable by an i64.
…ctions that must check if the memory offset plus the size of the type is within the bounds of our memory. In order for us to emit this code, that value must fit in a u64. In the case where the memory offset is u64::MAX, then our rust code panics in trying to generate the wasm due to an attempt to add with overflow. To solve this, I've added an upper bound on our memory offsets for load operations while running in non-trapping mode.
3af5bea
to
a42cabf
Compare
This was happening with modules that had a large number of memory.grow or table.grow instructions.
The only traps this is now throwing for me are StackOverflows on modules with recursive functions that look something like this:
These can't easily be avoided without somehow keeping track of the call-stack size, which we'd have to do in the emitted wasm, and then somehow handling the "we're about to exceed our stack depth" case. It takes the fuzzer a few hours to run into this case on my machine. From chatting with @fitzgen it sounds like ignoring this for now and documenting the behavior in the config docs should be good for the time being. If I add a case to ignore StackOverflow traps, like this: - Err(err) if err.to_string().contains("all fuel consumed") => continue,
+ Err(err)
+ if err.to_string().contains("all fuel consumed")
+ || err.to_string().contains("call stack exhausted") =>
+ {
+ continue
+ } then the fuzzer runs for 20+ hours on my machine! I think this means that we've found all the cases that we would expect to trap 🎉 @fitzgen any other changes you'd like to see here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks @itsrainy! Just one nitpick about documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This was a big one!
Prior to this change, the non-trapping logic in wasm-smith was implemented as a post-processing step. Random wasm would get generated and then the
no_traps()
logic would attempt to modify the generated instructions to ensure that the resulting wasm wouldn't trap. This led to a couple challenges: there are some instructions (such asunreachable
andcall_indirect
) that we don't want to emit at all in non-trapping mode, and there are some instructions that we haven't yet implemented a non-trapping solution for (such as SIMD Lane loads/stores). This meant that running wasm-smith in non-trapping mode could still result in a wasm program that would trap. While discussing how best to communicate and work around this nuance, @fitzgen and I realized that these were probably better dealt with while the code is being generated, rather than after the fact.This change moves much of the non-trapping logic from
src/core/no_traps.rs
to a code_builder module (src/core/code_builder/no_traps.rs
). It then calls into those functions from CodeBuilder, while entirely avoidingunreachable
,call_indirect
, and possibly trapping instructions that we haven't yet dealt with.TODO:
disallow_traps
works and to find the cases I inevitably missed